-
Notifications
You must be signed in to change notification settings - Fork 3
Add TooManyRequests error for 429 #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This will make it a bit easier to follow, instead of people having to remember that 429 is because of rate limiting. https://httpstatuses.com/429
lib/help_scout.rb
Outdated
@@ -206,6 +208,8 @@ def request(method, path, options) | |||
raise NotFoundError | |||
when HTTP_INTERNAL_SERVER_ERROR | |||
raise InternalServerError | |||
when HTTP_TOO_MANY_REQUESTS | |||
raise TooManyRequestsError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if we can return the Rate Limiting headers: http://developer.helpscout.net/help-desk-api/#rate-limiting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting idea, what would you like to return? If we've hit the limit, I don't know what number makes sense there. The remaining amount of requests will probably be 0.
We could add the amount of time to wait in the error message perhaps:
A "Retry-After" header is returned indicating how many seconds to wait until retry. Applicable to all requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always check the headers directly via client.last_response
, so if we just put in a 'nice' error message we at least give some feedback on the exception. Something a long the lines of "Rate limit (200 requests per minute) reached. You can try again after x seconds. See http://developer.helpscout.net/help-desk-api/#rate-limiting for more info". Or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed an update, let me know what you think.
Kind of hard to test this IRL, but I think this covers it. Will keep an eye on it and update if it does turn out to be different in production.
🚢, can you also release a new version of the gem? |
Sure thing, will push the new commit to master |
0.8.0: 0f166b2 |
This will make it a bit easier to follow, instead of people having to
remember that 429 is because of rate limiting.
https://httpstatuses.com/429